Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Wrap dyn type with parentheses in suggestion #120929

Merged
merged 1 commit into from
Apr 23, 2024
Merged

Wrap dyn type with parentheses in suggestion #120929

merged 1 commit into from
Apr 23, 2024

Conversation

long-long-float
Copy link
Contributor

Close #120223

Fix wrong suggestion that is grammatically incorrect.
Specifically, I added parentheses to dyn types that need lifetime bound.

help: consider adding an explicit lifetime bound
  |
4 |     executor: impl FnOnce(T) -> (dyn Future<Output = ()>) + 'static,
  |                                 +                       +++++++++++

@rustbot
Copy link
Collaborator

rustbot commented Feb 11, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 11, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@long-long-float
Copy link
Contributor Author

@fmease Could you review this PR? Thank you.

@fmease
Copy link
Member

fmease commented Feb 18, 2024 •
edited
Loading

Sure, I plan on reviewing this later today. Sorry for taking a bit, I'm assigned to several PRs and I continuously work on PRs myself ^^'

@bors
Copy link
Contributor

bors commented Feb 24, 2024

☔ The latest upstream changes (presumably #121549) made this pull request unmergeable. Please resolve the merge conflicts.

fmease
fmease requested changes Mar 6, 2024
View reviewed changes
Copy link
Member

@fmease fmease left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry for leaving you hanging like that! I have a couple of suggestions for how to structure this a bit differently and some correctness things.

At the moment your changes only trigger in very few cases. I think we should generalize it to apply to more + XXX-style suggestions.

First and foremost, I would check the other 3 usage sites bounds_span_for_suggestion whether they could benefit from this, too (I even suggest you to merge bounds_span_for_suggestion_with_parentheses into it in one of my review comments). You could even consider grep'ing through tests/ui/ to check if there are any other incorrect consider adding an explicit lifetime bound suggestions.

In summary, I'm a fan of fixing the incorrect diagnostic suggestion but I'd like to make sure to tap the full potential.

compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
tests/ui/suggestions/issue-120223.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 6, 2024
@long-long-float
Copy link
Contributor Author

@fmease Thank you for detailed review! I will fix them and check for more patterns in a couple of days.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@long-long-float
Copy link
Contributor Author

I will fix testing failure at first.

@fmease
Copy link
Member

fmease commented Mar 23, 2024

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2024
@long-long-float
Copy link
Contributor Author

@fmease

First and foremost, I would check the other 3 usage sites bounds_span_for_suggestion whether they could benefit from this, too.

I checked them:

  • compiler/rustc_hir_typeck/src/method/suggest.rs
    • tests/ui/suggestions/impl-trait-with-missing-trait-bounds-in-arg.rs
      • The impl Trait accepts impl Foo + Bar so we don't need parenthesis.
      • I think this suggestion is for type parameters (impl Trait in arguments), not dyn.
  • compiler/rustc_middle/src/ty/diagnostics.rs
    • Same as above?
  • compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs
    • ?Sized cannot be used in trait object types (dyn).

You could even consider grep'ing through tests/ui/ to check if there are any other incorrect consider adding an explicit lifetime bound suggestions.

I searched consider adding an explicit lifetime bound under tests/ui and couldn't find any incorrect suggestions.

I think incorrect suggestions which need parentheses happen with only dyn (I'm not familiar with Rust, so it may incorrect).

fmease
fmease reviewed Mar 30, 2024
View reviewed changes
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

Sign in to view
fmease
fmease requested changes Apr 19, 2024
View reviewed changes
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

First and foremost, I would check the other 3 usage sites bounds_span_for_suggestion whether they could benefit from this, too.

I could find an example for each usage site. Please make them use open_paren_sp as well, thanks! :)

After that, this PR should be good to go! One more thing, could you squash the commits into one (or fewer) commits?

tests/ui/suggestions/wrap-dyn-in-suggestion-issue-120223.rs Outdated Show resolved Hide resolved
tests/ui/suggestions/wrap-dyn-in-suggestion-issue-120223.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/method/suggest.rs Show resolved Hide resolved
compiler/rustc_hir_typeck/src/method/suggest.rs Show resolved Hide resolved
compiler/rustc_middle/src/ty/diagnostics.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Show resolved Hide resolved
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 19, 2024
@long-long-float
Copy link
Contributor Author

@fmease
Thank you for reviewing! I fixed to use open_paren_sp in all sites, added tests, and squashed commits. Could you check them?

@long-long-float
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 21, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@fmease
Copy link
Member

fmease commented Apr 21, 2024

#120929 (comment)

CI still has errors, please fix them. If you have any questions regarding the failures, don't hesitate to reach out and ask them!

@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 21, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@long-long-float
Copy link
Contributor Author

@fmease Thank you! All tests are passed.

@fmease
Copy link
Member

fmease commented Apr 23, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 23, 2024

📌 Commit 31e581e has been approved by fmease

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 23, 2024
@fmease fmease mentioned this pull request Apr 23, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2024
@bors
Auto merge of rust-lang#124295 - fmease:rollup-i3apkc6, r=fmease
c2f2db7 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#120929 (Wrap dyn type with parentheses in suggestion)
 - rust-lang#122591 (Suggest using type args directly instead of equality constraint)
 - rust-lang#122598 (deref patterns: lower deref patterns to MIR)
 - rust-lang#123048 (alloc::Layout: explicitly document size invariant on the type level)
 - rust-lang#123993 (Do `check_coroutine_obligations` once per typeck root)
 - rust-lang#124218 (Allow nesting subdiagnostics in #[derive(Subdiagnostic)])
 - rust-lang#124285 (Mark ``@RUSTC_BUILTIN`` search path usage as unstable)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 80f2b91 into rust-lang:master Apr 23, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2024
@rust-timer
Unrolled build for rust-lang#120929
888a0ad 
Rollup merge of rust-lang#120929 - long-long-float:wrap-dyn-in-suggestion, r=fmease

Wrap dyn type with parentheses in suggestion

Close rust-lang#120223

Fix wrong suggestion that is grammatically incorrect.
Specifically, I added parentheses to dyn types that need lifetime bound.

```
help: consider adding an explicit lifetime bound
  |
4 |     executor: impl FnOnce(T) -> (dyn Future<Output = ()>) + 'static,
  |                                 +                       +++++++++++
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmease fmease fmease approved these changes

Assignees

@fmease fmease

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.79.0
Development

Successfully merging this pull request may close these issues.

E0310 - Invalid suggestion when type is a function with a dyn return type
5 participants
@long-long-float @rustbot @rust-log-analyzer @fmease @bors